UDP protocol on connect agnhost command #98639

knabben · 2021-01-31T20:20:43Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Adding UDP support on agnhost connect subcommand, so we can have standardized tooling for e2e network probe tests.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
With a host listening on UDP 8000:

./agnhost serve-hostname --udp --http=false --port 8000

$ ./agnhost connect --protocol udp localhost:8000
$ echo $?
0

$ ./agnhost connect --protocol udp localhost:8002  # wrong port
REFUSED: read udp [::1]:51142->[::1]:8002: read: connection refused
$ echo $?
1

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2021-01-31T20:20:50Z

@knabben: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

aojea · 2021-01-31T20:46:46Z

test/images/agnhost/connect/connect.go

+		}
+	}
+
+	if _, err = conn.Write([]byte("hostname\n")); err != nil {


we can't assume the other end knows what hostname means,
this is the problem with UDP, it is connection less, so we need to send something and the other reply in order to know the connection is ok.
What if we add another flag to be able to send data and we default it to hostname or hello

cmd = []string{"/agnhost", "connect", fmt.Sprintf("%s:%d", addrTo, toPort), "--timeout=1s", "--protocol=udp","--data=hostname"}

sure, what about --udp-data, since it's only used for UDP?

yeah, sounds much better, but keep it defaulted with some data

aojea · 2021-01-31T20:48:18Z

test/images/agnhost/connect/connect.go

+		parseUDPErrorAndExit(err)
+	}
+
+	var buf = make([]byte, 1024)


we have to verify that we actually received some data

kubernetes/test/e2e/network/service.go

Lines 536 to 561 in bae644e

n, err := con.Read(buf)

if err != nil {

ret.Error = err

neterr, ok := err.(net.Error)

if ok && neterr.Timeout() {

ret.Status = UDPTimeout

} else if strings.Contains(err.Error(), "connection refused") {

ret.Status = UDPRefused

} else {

ret.Status = UDPError

}

framework.Logf("Poke(%q): %v", url, err)

return ret

}

ret.Response = buf[0:n]

if params.Response != "" && string(ret.Response) != params.Response {

ret.Status = UDPBadResponse

ret.Error = fmt.Errorf("response does not match expected string: %q", string(ret.Response))

framework.Logf("Poke(%q): %v", url, ret.Error)

return ret

}

ret.Status = UDPSuccess

framework.Logf("Poke(%q): success", url)

return ret

raising an error when readBytes == 0..
looks something that can be controlled on a flag i.e. --wait-response. or even use a --udp-request | --udp-response..

if you don't receive any bytes you can not known that the other side is receiving packets.
This is the same we discussed in slack regarding netcat, it just checks that the packet goes out, it doesn't mean it has been received, there is no ACK... 🤷

maybe you can return an error more descriptive "NO DATA RECEIVED" , and clarifiy that we can't guarantee the port is open

aojea · 2021-01-31T21:36:40Z

test/images/agnhost/connect/connect.go


 func init() {
 	CmdConnect.Flags().DurationVar(&timeout, "timeout", time.Duration(0), "Maximum time before returning an error")
-	CmdConnect.Flags().StringVar(&protocol, "protocol", "tcp", "The protocol to use to perform the connection, can be tcp or sctp")
+	CmdConnect.Flags().StringVar(&protocol, "protocol", "tcp", "The protocol to use to perform the connection, can be tcp, udp or sctp")
+	CmdConnect.Flags().StringVar(&udpData, "udp-data", "", "The UDP payload send to the server")


we should put some data as default, "hello" or "hostname" can be valid since are used in our tests in a number of cases, I think hostname is used in more places

aojea · 2021-02-01T08:14:09Z

/lgtm
/assign @claudiubelu @spiffxp

claudiubelu · 2021-02-01T10:33:24Z

test/images/agnhost/connect/connect.go

+	}
+
+	// ensure the response from UDP server
+	if readBytes == 0 {


So, basically, we just have to make sure that the UDP server sends us some data back, the content of which is unimportant, right?

I think this is the more general approach .... we can complicate it more and add an option to match the answer ... but if we don´t check that at least we receive something we can´t know if the port was open or not

more history about this https://nmap.org/book/scan-methods-udp-scan.html#:~:text=Fortunately%2C%20Nmap%20can%20help%20inventory,packet%20to%20every%20targeted%20port.

claudiubelu · 2021-02-02T12:09:50Z

/lgtm

dims · 2021-02-04T16:51:03Z

@knabben looks like we are adding a new "feature", please update test/images/agnhost/VERSION

aojea · 2021-02-04T17:36:43Z

yeah, I forget that, sorry, I think you have to bump these 3 files:

build/dependencies.yaml
test/images/agnhost/VERSION
test/images/agnhost/agnhost.go

knabben · 2021-02-04T17:53:58Z

I was about to ask what is the release process :) thanks @aojea @dims

dims · 2021-02-05T17:34:39Z

/hold

some more files need to be updated.

knabben · 2021-02-05T17:43:34Z

@dims bumped the files. Is this something that needs to be changed as well? https://github.com/kubernetes/kubernetes/blob/ead0271fcfbf0ee482e14151f338a6590e38e3f5/build/dependencies.yaml#L12

dims · 2021-02-05T17:48:47Z

build/dependencies.yaml

@@ -1,7 +1,7 @@
 dependencies:
  # agnhost: bump this one first
  - name: "agnhost"
-    version: "2.27"
+    version: "2.28"


Don't change this just yet (since this version does not exist in the gcr repository)

oh.. the dependencies job is failing with a test/images/agnhost/VERSION mismatch.

@dims I think that is correct

agnhost: bump this one first

there is another place that is the one that has to be bumped later, once the promotion has been done

dims · 2021-02-05T17:49:05Z

test/images/agnhost/agnhost.go

@@ -51,7 +51,7 @@ import (
 func main() {
 	rootCmd := &cobra.Command{
 		Use:     "app",
-		Version: "2.27",
+		Version: "2.28",


Same as above

I think is correct, this has to be bumped first so we can promote it later

aojea · 2021-02-07T20:44:36Z

🤔 it fails

hack/make-rules/../../hack/verify-vendor.sh
make: *** [Makefile:128: verify] Error 1
+ EXIT_VALUE=2
+ set +o xtrace

you can run it locally to find out what is the problem ...

knabben · 2021-02-07T20:53:57Z

locally the command pass, let me rerun this again here.

/test pull-kubernetes-dependencies

knabben · 2021-02-07T21:31:20Z

/test pull-kubernetes-bazel-test

dims · 2021-02-09T14:00:16Z

/hold cancel
/approve

please get an /lgtm from @aojea :) you main need additional approvals for the build/ directory

aojea · 2021-02-09T17:12:33Z

/lgtm
/assign @spiffxp

claudiubelu · 2021-02-09T17:14:19Z

/approve

spiffxp

/approve
/lgtm
Now that connect supports N=3 protocols, I think it'd be appropriate to refactor instead of continuing to copy-paste. It feels to me like there's plenty of untested room for subtle bugs between impls, but I won't block for that.

k8s-ci-robot · 2021-02-09T17:30:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: claudiubelu, dims, knabben, spiffxp

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~build/OWNERS~~ [spiffxp]
~~test/images/agnhost/OWNERS~~ [claudiubelu,dims,spiffxp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

aojea · 2021-02-09T17:33:15Z

Now that connect supports N=3 protocols, I think it'd be appropriate to refactor instead of continuing to copy-paste. It feels to me like there's plenty of untested room for subtle bugs between impls, but I won't block for that.

yeah, but the networking tests are a minefield, and this only probes for connectivity , i.e. I submit something the other side replays something, there are other tests that need to check the content of the replay, i.e. to verify that the the pod belong to the expected hostname, so this is no a replacement for those.

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 31, 2021

k8s-ci-robot requested review from dims and listx January 31, 2021 20:21

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 31, 2021

knabben force-pushed the udp-connect-agnhost branch from f61f298 to a9a87e3 Compare January 31, 2021 20:22

aojea reviewed Jan 31, 2021

View reviewed changes

knabben force-pushed the udp-connect-agnhost branch from a9a87e3 to 3450c18 Compare January 31, 2021 21:26

aojea reviewed Jan 31, 2021

View reviewed changes

knabben force-pushed the udp-connect-agnhost branch from 3450c18 to 3eb63b5 Compare January 31, 2021 21:49

k8s-ci-robot assigned claudiubelu, spiffxp and aojea Feb 1, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 1, 2021

claudiubelu reviewed Feb 1, 2021

View reviewed changes

knabben mentioned this pull request Feb 1, 2021

Adding UDP network policies tests #98636

Merged

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 5, 2021

knabben force-pushed the udp-connect-agnhost branch from 3eb63b5 to ead0271 Compare February 5, 2021 17:42

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2021

dims reviewed Feb 5, 2021

View reviewed changes

knabben force-pushed the udp-connect-agnhost branch from ead0271 to 79648f4 Compare February 5, 2021 17:57

UDP protocol on connect agnhost command

f1da110

knabben force-pushed the udp-connect-agnhost branch from 79648f4 to f1da110 Compare February 7, 2021 20:30

aojea mentioned this pull request Feb 8, 2021

Agnhost: make it possible to pass the addresses to listen on for udp #98883

Merged

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 9, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2021

spiffxp approved these changes Feb 9, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 9, 2021

k8s-ci-robot merged commit ab5a8c4 into kubernetes:master Feb 9, 2021

k8s-ci-robot added this to the v1.21 milestone Feb 9, 2021

knabben deleted the udp-connect-agnhost branch February 9, 2021 18:29

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 29, 2021

aojea mentioned this pull request Mar 30, 2021

test/e2e: fix the OIDC discovery test with ECDSA service account key #100680

Merged

danwinship mentioned this pull request Apr 21, 2024

[LinuxOnly] is egregiously misused at least in sig-network tests #124426

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UDP protocol on connect agnhost command #98639

UDP protocol on connect agnhost command #98639

knabben commented Jan 31, 2021 •

edited by liggitt

k8s-ci-robot commented Jan 31, 2021

aojea Jan 31, 2021

knabben Jan 31, 2021

aojea Jan 31, 2021

aojea Jan 31, 2021

knabben Jan 31, 2021

aojea Jan 31, 2021

aojea Jan 31, 2021

aojea Jan 31, 2021

aojea commented Feb 1, 2021

claudiubelu Feb 1, 2021

aojea Feb 1, 2021 •

edited

aojea Feb 8, 2021

claudiubelu commented Feb 2, 2021

dims commented Feb 4, 2021

aojea commented Feb 4, 2021

knabben commented Feb 4, 2021

dims commented Feb 5, 2021

knabben commented Feb 5, 2021

dims Feb 5, 2021

knabben Feb 5, 2021

aojea Feb 7, 2021 •

edited

dims Feb 5, 2021

aojea Feb 8, 2021

aojea commented Feb 7, 2021

knabben commented Feb 7, 2021

knabben commented Feb 7, 2021

dims commented Feb 9, 2021

aojea commented Feb 9, 2021

claudiubelu commented Feb 9, 2021

spiffxp left a comment

k8s-ci-robot commented Feb 9, 2021

aojea commented Feb 9, 2021

	n, err := con.Read(buf)
	if err != nil {
	ret.Error = err
	neterr, ok := err.(net.Error)
	if ok && neterr.Timeout() {
	ret.Status = UDPTimeout
	} else if strings.Contains(err.Error(), "connection refused") {
	ret.Status = UDPRefused
	} else {
	ret.Status = UDPError
	}
	framework.Logf("Poke(%q): %v", url, err)
	return ret
	}
	ret.Response = buf[0:n]

	if params.Response != "" && string(ret.Response) != params.Response {
	ret.Status = UDPBadResponse
	ret.Error = fmt.Errorf("response does not match expected string: %q", string(ret.Response))
	framework.Logf("Poke(%q): %v", url, ret.Error)
	return ret
	}

	ret.Status = UDPSuccess
	framework.Logf("Poke(%q): success", url)
	return ret

UDP protocol on connect agnhost command #98639

UDP protocol on connect agnhost command #98639

Conversation

knabben commented Jan 31, 2021 • edited by liggitt

k8s-ci-robot commented Jan 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Feb 1, 2021

Choose a reason for hiding this comment

aojea Feb 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claudiubelu commented Feb 2, 2021

dims commented Feb 4, 2021

aojea commented Feb 4, 2021

knabben commented Feb 4, 2021

dims commented Feb 5, 2021

knabben commented Feb 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea Feb 7, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Feb 7, 2021

knabben commented Feb 7, 2021

knabben commented Feb 7, 2021

dims commented Feb 9, 2021

aojea commented Feb 9, 2021

claudiubelu commented Feb 9, 2021

spiffxp left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 9, 2021

aojea commented Feb 9, 2021

knabben commented Jan 31, 2021 •

edited by liggitt

aojea Feb 1, 2021 •

edited

aojea Feb 7, 2021 •

edited